Skip to content

Conversation

@FGasper
Copy link
Collaborator

@FGasper FGasper commented Nov 27, 2024

Reviewer note

While this is a regrettably large changeset, most of it is trivial changes like passing a context where previously the function used context.Background().

The check logic contains most of this PR’s “interesting” changes.


In certain cases migration-verifier would log on fatal errors rather than propagating them back up the call stack. This has led to failures to compare documents that were effectively ignored save for being noted in the log. This changeset fixes that so that such exceptional cases will halt the verification.

Broadly speaking, this changeset:
a) brings idiomatic Context propagation in places where context.Background() was used
b) converts Error() logs and “failed” task statuses to returned errors

As of this changeset, any task in “failed” indicates a mismatch rather than a failure to complete the check.

A separate changeset will introduce a retryer to FetchAndCompareDocuments.

Other, specific changes:

  • CheckWorker() is rewritten to use an errgroup, which automatically cancels context if a thread fails.
  • CheckDriver() now returns errors rather than Error() logging them.
  • A few places’ direct checks for ctx.Done() are removed. (They’re superfluous.)
  • TestGenerationalRechecking and TestPartitionWithFilter no longer use constant DB names.
  • RunForUUIDErrorOnly() now takes a context. (NB: This method is currently unused.)
  • Most uses of context.Background() in tests are now suite.Context().
  • The unused RecheckTasks is removed from VerificationStatus.

@FGasper FGasper requested a review from tdq45gj November 27, 2024 01:16
@FGasper FGasper marked this pull request as ready for review November 27, 2024 01:24
@FGasper FGasper changed the title REP-5324 Normalize error handling, and add retries on document reads. REP-5324 Normalize error handling. Nov 27, 2024
Copy link
Collaborator

@tdq45gj tdq45gj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good overall % two comments.

@FGasper FGasper requested a review from tdq45gj November 27, 2024 15:09
@FGasper FGasper force-pushed the REP-5324-retry-failed-doc-comparison branch from ae8fc6f to 4c54c68 Compare November 27, 2024 17:38
@FGasper FGasper merged commit 4f3d3aa into mongodb-labs:main Nov 27, 2024
33 checks passed
@FGasper FGasper deleted the REP-5324-retry-failed-doc-comparison branch November 27, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants